Issue #50: HTTP errors are ignored and result in empty responses with…#55
Issue #50: HTTP errors are ignored and result in empty responses with…#55tdltdl wants to merge 1 commit intoipinfo:masterfrom
Conversation
…s with cache poisoning. WARNING This change is not a drop in replacement as if we get 403 because the token is incorrect, a checked exception is raised. Also note that, if an empty or null token is passed, it is now a fail fast (instead of getting a null pointer exception at the first request)
max-ipinfo
left a comment
max-ipinfo
left a comment
There was a problem hiding this comment.
@tdltdl thanks for sending your PR. I will continue to bring timely feedback from now on so we can create new package versions after each PR.
Please go over the exception restructuring suggested, with all HTTP exceptions inheriting from HttpErrorException. That includes simplifying all throws clauses to throws HttpErrorException where it makes sense.
Could you also update README.md' s Error section and document the new errors hierarchically?
I'll help you update the documentation for every exception after your next change.
| @@ -374,6 +375,9 @@ public Builder setCache(Cache cache) { | |||
| } | |||
|
|
|||
| public IPinfo build() { | |||
There was a problem hiding this comment.
Shouldn't this have throws IllegalArgumentException?
| * <p>That likely indicates that the token is either missing, incorrect or expired. | ||
| * It is a checked exception so, if you have multiple tokens (example during transition), you can fall back to another token.</p> | ||
| */ | ||
| public class InvalidTokenException extends Exception { |
There was a problem hiding this comment.
Now that we've introduced a proper HttpErrorException class, we should have InvalidTokenException and RateLimitedException also inherit from it, as they are technically exceptions stemming from an HTTP error response.
This helps simplify the design (no need to disambiguate everywhere with throws RateLimitedException, InvalidTokenException ) and it also helps clarify (in code) that they stem from the same source.
| } | ||
|
|
||
| public abstract T handle() throws RateLimitedException; | ||
| public abstract T handle() throws RateLimitedException, InvalidTokenException; |
There was a problem hiding this comment.
This can be simplified to throws HttpErrorException
| public abstract T handle() throws RateLimitedException, InvalidTokenException; | ||
|
|
||
| public Response handleRequest(Request.Builder request) throws RateLimitedException { | ||
| public Response handleRequest(Request.Builder request) throws RateLimitedException, InvalidTokenException { |
There was a problem hiding this comment.
Similarly, this can be simplified throws HttpErrorException
|
Why has this PR been closed without merge? This leads to cache poisoning... |
|
@tdltdl this PR has been stale for a very long time and the comments have not been addressed. I'll let @silvanocerza decide if he wants to revive this PR. Thanks for your understanding. |
|
In this case the maintainer didn't act on it for a long time. I am a bit surprised you want your Cust to have their cache poisoned with incorrect data. It also appears that strongly type exception to make API explicit and help Cus t know what they need to handle is not a design goal. |
2 participants
… cache poisoning.
WARNING This change is not a drop in replacement as if we get 403 because the token is incorrect, a checked exception is raised.
Also note that, if an empty or null token is passed, it is now a fail fast (instead of getting a null pointer exception at the first request)